-
Notifications
You must be signed in to change notification settings - Fork 517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[contrib] Refactor incidence_analysis
to cache a graph instead of a matrix
#2715
Conversation
…x, and remove some comments
win/3.9 teest failure appears unrelated |
It is unrelated. We're looking into this failure - it's been consistent across all new jobs run today. |
@mrmundt Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some small typos and questions that I have, but otherwise, this is looking great.
…al to match docstring
@Robbybp - There were some immense issues with our Jenkins server over the last 16ish hours. I'm hopeful that they are resolved now. Please ignore previous failures. |
Codecov ReportBase: 87.03% // Head: 86.98% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2715 +/- ##
==========================================
- Coverage 87.03% 86.98% -0.06%
==========================================
Files 771 771
Lines 86162 86246 +84
==========================================
+ Hits 74993 75019 +26
- Misses 11169 11227 +58
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
EDIT: Dependence of #2715, #2716, #2723, and #2727
plot
method toIncidenceGraphInterface
#2716 incorporates [contrib] Refactorincidence_analysis
to cache a graph instead of a matrix #2715plot
method toIncidenceGraphInterface
#2716 and Adding basic diagnostics output to solve_strongly_connected_components #2723In summary, merging #2727 will incorporate all the others, but they are broken up to be easier to review.
Summary/Motivation:
IncidenceGraphInterface
accepts a model and caches the incidence matrix (in COO format) of variables and constraints. This allows callingdulmage_mendelsohn
, etc. without re-constructing the matrix, but does not allow fast look-ups of "adjacent" constraints and variables, i.e. getting all constraints that contain a variable or all variables that participate in a constraint. This would require storing the incidence matrix in both CSR and CSC formats, or alternatively storing the bipartite incidence graph as an adjacency list. This PR refactorsincidence_analysis
to cache the bipartite incidence graph as a NetworkX Graph instead of caching a SciPy COO matrix, allowing aget_adjacent_to
method that quickly looks up adjacent variables/constraints.Changes proposed in this PR:
incidence_graph
attribute onIncidenceGraphInterface
incidence_matrix
attribute is now a "property" that computes the matrix from the cached graph. This means that this attribute still works, but is now an expensive operation.dulmage_mendelsohn
, and the underlying implementations so we do not convert the cached graph to a COO matrix, just to convert it back to a graphget_adjacent_to
methodThis PR also runs Black on
incidence_analysis
, for good measure.Next steps:
This PR sets the stage for several improvements I'd like to make to
incidence_analysis
, which will be subsequent PRs:IncidenceGraphInterface
IncidenceGraphInterface
. I would also like to update theblock_triangularize
andget_diagonal_blocks
method names and APIs, as I think the maps returned byblock_triangularize
are confusing, and the nameget_diagonal_blocks
is an incomplete misnomer.Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: